Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.56%. Comparing base (1e16207) to head (60d663f).
Report is 8 commits behind head on main.

@@            Coverage Diff             @@
##             main     #357      +/-   ##
==========================================
+ Coverage   67.52%   67.56%   +0.03%     
==========================================
  Files         114      114              
  Lines        5916     5920       +4     
==========================================
+ Hits         3995     4000       +5     
+ Misses       1921     1920       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I was looking at this again and realized that the 'PA' quantity is in the input datamodel as meta.aperture.position_angle, but that isn't carried into the output AmiOiModel, I'm guessing because it's not in the primary extension? I would also guess it's not best practice to have the same value defined as two different keywords in the dictionary, so I'm wondering if there's a better way to include it in the schema for oifits files?

I converted this back to draft until the details of PA can be resolved.

I was looking at this again and realized that the 'PA' quantity is in the input datamodel as meta.aperture.position_angle, but that isn't carried into the output AmiOiModel, I'm guessing because it's not in the primary extension? I would also guess it's not best practice to have the same value defined as two different keywords in the dictionary, so I'm wondering if there's a better way to include it in the schema for oifits files?

Thanks for giving this another look. Does PA_APER (at meta.aperture.position, the position angle of the aperture) contain the value that you would have carried over to PA (the observation position angle)?

I think 'meta.pointing.pa_v3' from the calints file (FITS keyword PA_V3) is actually the one we want. There are a few similar values in the header but that's what we've been using. If it's not too much trouble, let's also add 'meta.wcsinfo.v3yangle' (FITS keyword V3I_YANG) because we use that as well. The original reason for defining the separate PA keyword was because we actually use an angle of PA_V3 - V3I_YANG, but at some point settled on just storing the plain PA_V3 value in the OIFITS.
In summary, I think we should remove the unused 'PA' keyword from the keyword dictionary in favor of just using these two existing ones.

It sounds like referencing and using the keywords defined in wcsinfo:
https://github.com/spacetelescope/stdatamodels/blob/main/src/stdatamodels/jwst/datamodels/schemas/wcsinfo.schema.yaml
would provide the ones you need (and be consistent with other data products). We would need to check:

• if this is sufficient. Are there other keywords you expect to use from the input?
• if the keywords in wcsinfo conflict at all with oifits

How about we pair this PR with yours on jwst: spacetelescope/jwst#8846 by changing the stdatamodels dependency in the pyproject.toml in your PR to point to the source branch for this PR?

"stdatamodels @ git+https://github.com/braingram/stdatamodels.git@calib_pa",

That way the CI and regtests will pick up the changes in this PR when testing your PR. If you run into issues where a keyword isn't available I can add it and push to this PR. Once they're both ready we can open them for review with a note that this PR will have to be merged first and then your jwst PR updated to point back to stdatamodels main.

As far as I am aware right now, those keywords defined in wcsinfo should be sufficient to add to the output AmiOiModel. I also don't see any conflicting keywords in the OIFITS standard or this document that describes more standardized keywords, including some WCS ones. I just pushed the modified pyproject.toml file on my jwst PR so hopefully we can get the remaining failing tests to pass.

Thanks! I updated the schema to contain wcsinfo. It may require some updates to the jwst code to copy the data over from the input to the output files.

Let me know what updates would be helpful for this PR and if there's anything I can do to help with the jwst one.

Thanks! I made some updates to carry the wcsinfo through to the output file, and that seems to be working fine, as is the calibrator_object_id in the calibrated products. I also tested the code using the wcsinfo.roll_ref keyword in place of the pointing.pa_v3 and the end product was still good. Out of curiosity, is it possible to add individual keywords from other schemas, or does it have to be a whole schema (the way you added wcsinfo to the new ami schema)?

From my end I think we're good to merge this so we can move forward with getting the JWST PR(s) ready to merge as well.

Hi Brett, when Melanie ran the reg tests for spacetelescope/jwst#8846 she found that the oi.fits products now contain the SCI extension, which they shouldn't. I'm guessing it's because we're now copying the wcsinfo over, and that is associated with the SCI extension of the input. Is there a way of just taking the header keywords without the data part of the HDU?

The inclusion of the SCI extension is required for pulling in wcsinfo since that's where the PA_V3 (and other keywords) are defined:

WARNING! Expecting format 32A but found 16A for column 'TARGET'
WARNING! Expecting format 32A but found 16A for column 'SPECTYP'
OIFITS version 2 data:
  ORIGIN  = 'STSCI'
  DATE    = '2024-11-22T20:48:13.854'
  DATE-OBS= '2022-06-05'
  TELESCOP= 'JWST'
  INSTRUME= 'NIRISS'
  OBSERVER= 'Thatte, Deepashri'
  OBJECT  = 'AB-DOR'
  INSMODE = 'NRM'
  OBSTECH = ''

  1 OI_ARRAY tables:
    #1  ARRNAME='jwst_g7s6c'  7 elements
  1 OI_WAVELENGTH tables:
    #1  INSNAME='NIRISS'  1 channels   4817.3- 4817.3nm
  0 OI_CORR tables:
  0 OI_INSPOL tables:
  1 OI_VIS tables:
    #1  DATE-OBS=2022-06-05
    INSNAME='NIRISS'  ARRNAME='jwst_g7s6c'  CORRNAME=''
        21 records x   1 wavebands
  1 OI_VIS2 tables:
    #1  DATE-OBS=2022-06-05
    INSNAME='NIRISS'  ARRNAME='jwst_g7s6c'  CORRNAME=''
        21 records x   1 wavebands
  1 OI_T3 tables:
    #1  DATE-OBS=2022-06-05
    INSNAME='NIRISS'  ARRNAME='jwst_g7s6c'  CORRNAME=''
        35 records x   1 wavebands
  0 OI_FLUX tables:
All checks passed

Also, in that file the SCI exension contains no data:

No.    Name      Ver    Type      Cards   Dimensions   Format
  0  PRIMARY       1 PrimaryHDU     342   ()
  1  SCI           1 ImageHDU        31   ()
...

Are there other files where the SCI extension has data?

I guess there isn't technically an issue, it just seemed odd to have an extension with no data that doesn't really serve a purpose other than having the header keywords, especially since the 'SCI' extension in all the other pipeline product levels does contain the science data. If there's no way around it we can deal with it.
In retrospect I guess the original 'PA' keyword we added would have accomplished this, but there are already so many similar position angle-related values that it seemed messy to add another. On the other hand I didn't realize that each chunk of header keywords was tied to a particular extension by its schema and that the data part of the extension had to come along for the ride. There's no way to grab individual keywords from other schemas and just put them where we want is there? 🥲

After a little more consideration I think this tips the balance back in favor of using the 'PA' keyword we initially defined. Since the JWST keyword dictionary includes how it is derived, I don't think it's unreasonable or too confusing to include it despite its similarity to other keywords.
Will it work to revert this branch back to 03709e2, and then I can test populating 'telescope_roll` in the AMI code? I'm sorry for all this back and forth, I am slowly but surely starting to understand how the FITS files and various schemas work together.

I guess there isn't technically an issue, it just seemed odd to have an extension with no data that doesn't really serve a purpose other than having the header keywords, especially since the 'SCI' extension in all the other pipeline product levels does contain the science data. If there's no way around it we can deal with it. In retrospect I guess the original 'PA' keyword we added would have accomplished this, but there are already so many similar position angle-related values that it seemed messy to add another. On the other hand I didn't realize that each chunk of header keywords was tied to a particular extension by its schema and that the data part of the extension had to come along for the ride. There's no way to grab individual keywords from other schemas and just put them where we want is there? 🥲

Thanks. We could put PA_V3 in the primary extension by defining it in the ami.schema (the same as we had PA previously). We'd have to redefine each keyword you want to use from wcsinfo.schema and the produced files would differ from other data products (so for an amioi file the wcsinfo would be in a different extension). My initial impression of this is that it's more confusing than using the same keywords as the other data products (and including a SCI extension).

Perhaps @tapastro or @melanieclarke can weight in on this. I'm in favor of keeping consistency for keywords that are shared with other data products.

After a little more consideration I think this tips the balance back in favor of using the 'PA' keyword we initially defined. Since the JWST keyword dictionary includes how it is derived, I don't think it's unreasonable or too confusing to include it despite its similarity to other keywords. Will it work to revert this branch back to 03709e2, and then I can test populating 'telescope_roll` in the AMI code? I'm sorry for all this back and forth, I am slowly but surely starting to understand how the FITS files and various schemas work together.

If we go down that route, what other keywords from wcsinfo will be needed? #357 (comment) mentions V3I_YANG as well as PA_V3.

Perhaps @tapastro or @melanieclarke can weight in on this. I'm in favor of keeping consistency for keywords that are shared with other data products.

I am in favor of keeping keywords names and definitions consistent wherever possible, but I don't see any reason why they should always have to live in the same extension, especially if you have to make a whole new extension just to store them! From a FITS user perspective, it's very confusing to have to access an empty image extension to get keywords that are actually relevant to other extensions. I think it makes sense to copy the definition for the keywords needed from the wcsinfo schema, but change the HDU requirement to one that's already present in the data.

I think if we can copy the names and definitions of just the few keywords we need from wcsinfo into the ami.schema that would be ideal. That way we won't have the potentially confusing empty SCI extension or the additional keyword(s) used only for AMI with similar definitions to existing ones. We should just need ROLL_REF, V3I_YANG, and VPARITY. If they can be included in the primary header that would probably make the most sense.

I think if we can copy the names and definitions of just the few keywords we need from wcsinfo into the ami.schema that would be ideal. That way we won't have the potentially confusing empty SCI extension or the additional keyword(s) used only for AMI with similar definitions to existing ones. We should just need ROLL_REF, V3I_YANG, and VPARITY. If they can be included in the primary header that would probably make the most sense.

Thanks. Is PA_V3 not needed?

These changes will require updates to the keyword dictionary since none of those keywords are currently defined in the PRIMARY extension. Additionally we need to define "paths" and attribute names for these keywords. To try and summarize what needs to be determined here's a table with blank cells for items that need decisions. It's assumed all keywords will be in the PRIMARY header and any attributes not referenced in the table (like "title", etc) will be copied from wcsinfo.schema.

To provide an example VPARITY is currently in wcsinfo and the attribute name is vparity. This gives a path of meta.wcsinfo.vparity. There are existing unresolved "path" discrepancies between the keyword dictionary and datamodel schemas that makes matching the path unlikely in the near term. However this path will need to be known to add these keywords to the schema. To avoid confusion I suggest we pick paths that differ from the keywords currently stored in SCI (so not meta.pointing or meta.wcsinfo).

Thanks. Is PA_V3 not needed?

That's right, after some discussion on our team we decided we should use ROLL_REF instead of PA_V3, so PA_V3 is no longer needed.

These changes will require updates to the keyword dictionary since none of those keywords are currently defined in the PRIMARY extension. Additionally we need to define "paths" and attribute names for these keywords. To try and summarize what needs to be determined here's a table with blank cells for items that need decisions. It's assumed all keywords will be in the PRIMARY header and any attributes not referenced in the table (like "title", etc) will be copied from wcsinfo.schema.

I see, so there will essentially be duplicate entries in the keyword dictionary but with different paths. If possible I think it makes sense to put them in the same place as the other AMI-specific keywords. In the tree it looks like that's meta.oifits and appears in section "NIRISS AMI Information" in the keyword dictionary. Would that mean adding them to oifits.schema?

To fill out the blanks in the table:

That's right, after some discussion on our team we decided we should use ROLL_REF instead of PA_V3, so PA_V3 is no longer needed.

Thanks! Good to know.

I see, so there will essentially be duplicate entries in the keyword dictionary but with different paths. If possible I think it makes sense to put them in the same place as the other AMI-specific keywords. In the tree it looks like that's meta.oifits and appears in section "NIRISS AMI Information" in the keyword dictionary. Would that mean adding them to oifits.schema?

Since these aren't required or used by oifits how about putting them under ami? It looks like that's already the location for CALIB in the keyword dictionary (so maybe we should also update this PR to change meta.calibrator_object_id to meta.ami.calib). Is CALIB is still needed (I'm assuming yes but figured I'd confirm)?

That would make the table:

Since these aren't required or used by oifits how about putting them under ami?

Sure, that makes a lot of sense.

Is CALIB is still needed (I'm assuming yes but figured I'd confirm)?

Yep, we do still need calib. I think calibrator_object_id is more descriptive than calib (I just went with that because of the FITS key character limit), so if it's not an issue to have the attribute name be different from the keyword then I think keeping calibrator_object_id is a little clearer. Unless there's an aspect of this that I'm not grasping (quite probable)?

Since these aren't required or used by oifits how about putting them under ami?

Sure, that makes a lot of sense.

Is CALIB is still needed (I'm assuming yes but figured I'd confirm)?

Yep, we do still need calib. I think calibrator_object_id is more descriptive than calib (I just went with that because of the FITS key character limit), so if it's not an issue to have the attribute name be different from the keyword then I think keeping calibrator_object_id is a little clearer. Unless there's an aspect of this that I'm not grasping (quite probable)?

Thanks! I think we have an updated "game plan" now. I'll stick with calibrator_object_id. The keyword dictionary entry for this keyword will need to be updated but given the number of other required keyword dictionary changes this seems minor.

@rcooper295 The updates have been made to this PR. Let me know if there's anything else you need.

Thanks so much! I was able to make the changes in my code to use the updates and check that everything worked as expected, so I'm good with this.

I reran the regtests (same link as above). All failures are unrelated so merging.